storage/static.go: storage backend should not explicitly lower-case email ids. #1046
Conversation
storage/static.go
Outdated
| p.Email = strings.ToLower(p.Email) | ||
| passwordsByEmail[p.Email] = p | ||
| //Enable case insensitive email comparison. | ||
| lowerEmail = strings.ToLower(p.Email) |
rithujohn191
force-pushed
the
static-password-fix
branch
3 times, most recently
from
f103da8 to
d0dfe26
Compare
|
added a test and documentation |
storage/memory/static_test.go
Outdated
| if err := s.CreatePassword(p3); err != nil { | ||
| return err | ||
| } | ||
| return s.CreatePassword(p4) |
There was a problem hiding this comment.
Need to read this value back an ensure the emails match.
There was a problem hiding this comment.
This test makes sure that "[email protected]" cannot be created, so it ensures an error is returned
There was a problem hiding this comment.
do you want me to check if "[email protected]" gets created with the all lower case?
There was a problem hiding this comment.
Something like
p, err := s.GetPassword(strings.ToLower(p4.Email))
p.Email == p4.Email
storage/static.go
Outdated
| passwordsByEmail[p.Email] = p | ||
| //Enable case insensitive email comparison. | ||
| lowerEmail := strings.ToLower(p.Email) | ||
| passwordsByEmail[lowerEmail] = p |
There was a problem hiding this comment.
Could we log or something if the lowered email as already in the map?
There was a problem hiding this comment.
We would just return an error here. @rithujohn191 what do you think?
There was a problem hiding this comment.
Actually I only think we can log this error. Erroring out would be backward incompatible, if sensible :(
There was a problem hiding this comment.
So if we don't error out we essentially allow the user to overwrite the map. For the following ConfigMap:
staticPasswords:
- email: "[email protected]"
# bcrypt hash of the string "password"
hash: "$2a$10$2b2cU8CPhOTaGrs1HRQuAueS7JTT5ZHsHSzYiFPm1leZck7Mc8T4W"
username: "admin"
userID: "08a8684b-db88-4b73-90a9-3cd1661f5466"
- email: "[email protected]"
# bcrypt hash of the string "password"
hash: "$2a$10$2b2cU8CPhOTaGrs1HRQuAueS7JTT5ZHsHSzYiFPm1leZck7Mc8T4W"
username: "Admin"
userID: "08a8684b-db88-4b73-90a9-3cd1661f5466"
Only the second password gets registered. And then if the user tries to login with the first email it still succeed but returns the wrong email id in the claims:
"email": "[email protected]",
That's not desirable behavior :(
There was a problem hiding this comment.
@rithujohn191 right, but prior to this that config would not have errored, if we error now we might break someone during a dex upgrade.
There was a problem hiding this comment.
logged the error
rithujohn191
force-pushed
the
static-password-fix
branch
from
d0dfe26 to
294462e
Compare
|
working on adding the logger |
rithujohn191
force-pushed
the
static-password-fix
branch
from
294462e to
fd4f57b
Compare
|
Done :) |
storage/static.go: storage backend should not explicitly lower-case email ids.
No description provided.